Ensure close_notify is sent after user_canceled during quiet shutdown - #11225
Ensure close_notify is sent after user_canceled during quiet shutdown#11225holtrop-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR ensures that when a user_canceled alert is sent, a subsequent close_notify is still emitted during quiet shutdown, aligning shutdown behavior with the protocol expectation described in the TLS spec and addressing #11131.
Changes:
- Track whether a
user_canceledhas been sent (or queued) so quiet shutdown can still send the requiredclose_notify. - Update
wolfSSL_shutdown()quiet-shutdown path to flush/emitclose_notifywhenuser_canceledoccurred. - Add a regression test covering quiet shutdown behavior after
wolfSSL_SendUserCanceled().
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
wolfssl/internal.h |
Adds an options bit to record user_canceled state relevant to shutdown behavior. |
src/ssl_api_rw.c |
Implements the quiet-shutdown exception to still send close_notify after user_canceled. |
src/ssl.c |
Initializes the new options bit during SSL object reset/init. |
tests/api/test_ssl_rw.[ch] |
Registers and adds a new test validating the quiet shutdown + user_canceled behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
retest this please (build removed) |
|
retest this please (fatal: early EOF |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11225
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| if (!wolfssl_shutdown_flush_alert(ssl, &ret)) { | ||
| (void)wolfssl_shutdown_send_close_notify(ssl, &ret); | ||
| } | ||
| if (ret == WC_NO_ERR_TRACE(WOLFSSL_SHUTDOWN_NOT_DONE)) { |
There was a problem hiding this comment.
Quiet-shutdown user_canceled branch returns WOLFSSL_FATAL_ERROR with no error recorded · Incorrect error handling
When wolfssl_shutdown_send_close_notify() takes no action (isClosed, connReset, or sentNotify already set) it returns 0 and leaves *ret untouched, so the new quiet branch falls through with ret still at its WOLFSSL_FATAL_ERROR initializer while ssl->error stays WOLFSSL_ERROR_NONE — the caller gets a failure with nothing to query. The non-quiet branch at lines 1075-1102 records SOCKET_PEER_CLOSED_E for exactly this case; the quiet branch has no equivalent. It also makes a repeat wolfSSL_shutdown() return failure in builds where the wolfSSL_clear() reset of sentUserCanceled is not compiled in.
Related known finding #10650 (similar but distinct): Both affect wolfSSL_shutdown's quiet-shutdown path and close-notify handling, but #10650 faults by suppressing a required TLS 1.3 close_notify while this finding returns an unrecorded fatal error after the send helper takes no action. The root causes and required patches differ.
Fix: Decide ret explicitly in the quiet branch when the helpers leave it undecided: return WOLFSSL_SUCCESS when nothing remains to send, or record an error before returning failure.
| /* A "user_canceled" alert has gone out so we need a "close_notify" to | ||
| * follow it per RFC 9846 Section 6.1. */ | ||
| if (!wolfssl_shutdown_flush_alert(ssl, &ret)) { | ||
| (void)wolfssl_shutdown_send_close_notify(ssl, &ret); |
There was a problem hiding this comment.
Quiet-shutdown user_canceled path returns WOLFSSL_FATAL_ERROR with no error code set · TLS protocol issues
In the new quietShutdown && sentUserCanceled branch, both helpers are no-ops when sentNotify is already set or isClosed/connReset is set, so ret keeps its initial WOLFSSL_FATAL_ERROR (line 1029) while ssl->error is left untouched. Callers get -1 with wolfSSL_get_error() returning 0. This is adjacent to known finding #10650 (which this PR fixes) but is a distinct defect in the new branch's no-op path.
Related known finding #10650 (similar but distinct): Both are in wolfSSL_shutdown's quiet-shutdown handling, but #10650 concerns omission of a required close_notify; this finding concerns the new user_canceled no-op path retaining an initial fatal return without setting ssl->error. The faulting operations, root causes, and required patches differ.
Fix: Handle the no-op case explicitly: return WOLFSSL_SUCCESS when close_notify was already sent, and set ssl->error = SOCKET_PEER_CLOSED_E as the non-quiet path does when the peer closed.
Description
Ensure close_notify is sent after user_canceled during quiet shutdown
Fixes #11131
Testing
How did you test?
Checklist